Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TorqueScript unit test #592

Closed
wants to merge 4 commits into from
Closed

Conversation

Areloch
Copy link
Contributor

@Areloch Areloch commented Mar 24, 2014

Adds a unit test to the engine that tests some basic functionality of TorqueScript.

Can easily be extended later to test more things, but this acts as a solid starting point.

@Areloch Areloch changed the title Ts unit test TorqueScript unit test Mar 24, 2014
@crabmusket
Copy link
Contributor

I wonder whether these tests should be written in TS rather than in C++. It's cool that we have a C++ test framework set up, but I wonder what it gives us over the alternative.

@lukaspj
Copy link
Contributor

lukaspj commented Mar 24, 2014

@eightyeight I guess it wouldn't be unit-tests then but rather implementation tests.
What I mean is if we do the tests in TS, a bunch of other things could go wrong rather than the method we are testing, it could be script parsing, compilation, dictionary look-ups. These things are fine to test for as well, but should have their own unit-tests and not be covered by these tests.

Thats IMO ofc.

Edit: oh my bad, I thought this called the direct methods of the SimObject and not evaluated statements, guess I skimmed it a little too fast, then yes I agree it could have been written in TorqueScript as well.

@crabmusket
Copy link
Contributor

@lukaspj That's a good point, and a good distinction to make. Maybe we should be testing everything we can in TS, and everything we can't (i.e. the SimDictionary) in C++. But yes, tests in TS should probably be classed as integration tests, unless you're testing units that are themselves written in TS.

@Areloch
Copy link
Contributor Author

Areloch commented Mar 24, 2014

It could have been written in TS, but between possible cross-contamination of other issues that can give false-positives, you also need to consider that means we'd have to start including them in the templates somewhere.

That MAY not be too much of an issue when we get modular templates in, you can just drop in test files and whatnot, but currently that means you're looking to bloat the templates with non-game scripts.
Integrating them into the engine bypasses cross contamination issues as well as template bloat.

Just my thoughts on the approach to it.

@crabmusket
Copy link
Contributor

Looking at it in that light, this route involves bloating the engine source/executable itself with non-game-related code. Admittedly, a tiny amount, but the amount will grow. Or does the existing infrastructure ensure that unit test code is removed from release builds?

I thought about the false-positives angle, but I can't actually think of a case where changes could break useful scripts and not break unit test scripts.

@Areloch
Copy link
Contributor Author

Areloch commented Mar 25, 2014

Hm, you may be right about the whole false-positive thing. I looked at it from a 'test it close to the source' angle to minimize things breaking, but it may not matter over-all.

As for the bloat issue, in my mind, the templates have enough excess crap they don't need as it is, and the impact the unit tests have on the engine size is functionally non-existent unless we had a TON of them. We have to put the tests somewhere, and it impacts things less if it's in the engine.

To wit, the TS unit test is currently constrained to a single file that compiles into the engine and stops impacting the end product. Whereas to get the same result in script currently, the TASman setup is using 2 script files and added script exec dependencies. (Not ragging on it, just using it as the counter-example) They do the same job, but the level of 'bloat' is different - at least to me.

@dottools
Copy link

Unit tests should only ever be compiled and used when they're explicitly enabled no matter what build type is used. Might need to make this a projectGenerator option like we do for all the other optional components. That way when somebody really does want to run some unit tests all they need to do is run the project manager and generate themselves a unitTest variant of their projects.

@Areloch
Copy link
Contributor Author

Areloch commented Mar 25, 2014

@dottools
Excellent point. Currently, I'm pretty sure that it's just compiled in. Having the unit tests added as a component to the project would be really smart.

@crabmusket
Copy link
Contributor

That's where I was going with that question, but yes, they should definitely be in their own PG module.

As for script testing in scripts, I'm on the fence. I definitely agree that the current templates should not include TS unit tests in TS a la Tasman, but I didn't really imagine we'd stick unit tests in until we were refactoring the templates anyway.

Maybe this basic level of unit testing the TS interpreter should be done engine-side, and then obviously when we get to writing script functionality then tests will be in TS. It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle. That may be infeasible, though.

@Areloch
Copy link
Contributor Author

Areloch commented Mar 25, 2014

@eightyeight

Heh, we could always just set up a unit test template module with the unit test project component. That should cover everything ;)

@LuisAntonRebollo
Copy link
Contributor

We need a TS unit test. Good work.

@eightyeight, It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle

+1. I think it's better to move the test to c++

@crabmusket
Copy link
Contributor

@Areloch let's move ahead then with writing unit tests in C++ for the script engine itself. We should probably start a new branch in one of our repositories.

@crabmusket
Copy link
Contributor

@Areloch I just realised this file is full of tabs :P.

@crabmusket crabmusket closed this May 5, 2014
@crabmusket crabmusket reopened this May 5, 2014
@crabmusket
Copy link
Contributor

Whoops, no need to close. You could always rebase this branch to switch to spaces. While you're at it, maybe squash some of those minor commits! In fact this whole PR should probably be one commit.

@crabmusket crabmusket modified the milestones: 3.6, 3.7 Jul 11, 2014
@crabmusket
Copy link
Contributor

Closing for now as it doesn't merge cleanly. Will probably crib the code, though, for #866.

@crabmusket crabmusket closed this Oct 25, 2014
@Areloch Areloch deleted the TSUnitTest branch July 10, 2017 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants